Skip to content

Relocate tests in tests/ui #140029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reddevilmidzy
Copy link
Contributor

Part of #133895

Moved tests from a top-level directory into a more appropriate subdirectory.
If there is anything else that could be improved, please let me know!

r? jieyouxu

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 19, 2025
@reddevilmidzy reddevilmidzy changed the title Move test Relocate tests in tests/ui Apr 19, 2025
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! So, the new locations and names for these tests look good -- however, I'd like to see some short doc comments on the test intentions (they were not very obvious to me).

Please also squash the commits into one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: I honestly have no idea what this test is actually trying to exercise. This goes all the way back to 2010 d6b7c96.

I assume this is trying to exercise some short-circuiting behavior of || and && but the test itself makes it hard to tell.

EDIT: the OG version of this Rust was

fn incr(mutable &int x) -> bool {
  x += 1;
  check (false);
  ret false;
}

fn main() {

  auto x = (1 == 2) || (3 == 3);
  check (x);

  let int y = 10;
  log x || incr(y);
  check (y == 10);

  if (true && x) {
    check (true);
  } else {
    check (false);
  }

}

so yeah, I believe this is indeed a || and && short-circuiting test. It has a no-arg closure, presumably because back then there must've been a case where the parser confused || and the no-arg closure form || EXPR or sth.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's amusing is that this is one of the listings of the archive of OG rust syntax listed at https://brson.github.io/archaea/lazy-and-or.html/.

EDIT: I realized you can actually see the dynamic evolution of this test on that listing!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the analysis and sharing that interesting link! Now I understand that this test verifies short-circuiting behavior of logical operators.

I'll update the test with clearer naming and comments to better express its purpose while preserving the original functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And thanks for the historical context! That's super helpful 😁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: so this one is also introduced in the 2010 populate tree commit, and it kinda has been preserved all these years modulo some language changes. I believe this is some kind of "positive" smoke test of a tree of owned memory...?

Originally (like back in Rust 0.8), this was

tag list {
  cons(int,@list);
  nil;
}

fn main() {
  cons(10, @cons(11, @cons(12, @nil)));
}

This is also one of the OG rust syntax examples at https://brson.github.io/archaea/list.html.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remark: yeah, this test is because unary negation on an owned string didn't use to properly typeck, back in de4b383.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 19, 2025
@jieyouxu
Copy link
Member

I'm rolling a second compiler reviewer for a second opinion on "what on earth are these tests actually testing".

r? jieyouxu
r? compiler

@rustbot

This comment was marked as off-topic.

@rust-log-analyzer

This comment has been minimized.

@reddevilmidzy reddevilmidzy requested a review from jieyouxu April 19, 2025 13:51
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants